New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

JNI: Pass names of children struct columns to native Arrow IPC writer [skip ci] #7598

Merged

rapids-bot merged 16 commits into rapidsai:branch-0.19 from firestarman:arrow_ipc_column_metadata

Mar 20, 2021

Contributor

firestarman commented Mar 15, 2021 •

edited

Loading

This PR is to add the support of building the structure of column metadata from the flattened column names according to the table schema.
Since the children column metadata is required when converting cudf tables to arrow tables.

Also updating the related unit tests.

closes #7570

Signed-off-by: Firestarman firestarmanllc@gmail.com


          Pass names of children struct columns to native

1c8d676

Pass the names of children struct columns to the naitve for arrow
IPC writer, which is required to build column_metadata.

Also add the related unit tests.

Signed-off-by: Firestarman <firestarmanllc@gmail.com>

github-actions bot added the Java label

firestarman changed the title ~~JNI: Pass the names of children struct columns down to the native Arrow IPC writer.~~ JNI: Pass the names of children struct columns down to the native Arrow IPC writer [skip ci].

codecov bot commented Mar 15, 2021 •

edited

Loading

Codecov Report

Merging #7598 (16fa512) into branch-0.19 (7871e7a) will increase coverage by 0.52%.
The diff coverage is 93.22%.

❗ Current head 16fa512 differs from pull request most recent head 54395f0. Consider uploading reports for the commit 54395f0 to get more accurate results

@@               Coverage Diff               @@
##           branch-0.19    #7598      +/-   ##
===============================================
+ Coverage        81.86%   82.38%   +0.52%     
===============================================
  Files              101      101              
  Lines            16884    17350     +466     
===============================================
+ Hits             13822    14294     +472     
+ Misses            3062     3056       -6

Impacted Files	Coverage Δ
python/cudf/cudf/core/index.py	`93.34% <ø> (+0.48%)`	⬆️
python/cudf/cudf/core/column/column.py	`87.83% <83.33%> (+0.07%)`	⬆️
python/cudf/cudf/core/column/numerical.py	`94.85% <85.71%> (-0.17%)`	⬇️
python/cudf/cudf/core/frame.py	`89.12% <89.47%> (+0.10%)`	⬆️
python/cudf/cudf/core/column/decimal.py	`92.75% <90.32%> (-2.12%)`	⬇️
python/cudf/cudf/core/dataframe.py	`90.58% <95.00%> (+0.11%)`	⬆️
python/cudf/cudf/core/series.py	`91.57% <95.55%> (+0.78%)`	⬆️
python/cudf/cudf/core/column/string.py	`86.76% <100.00%> (+0.26%)`	⬆️
python/cudf/cudf/core/column_accessor.py	`95.45% <100.00%> (+0.14%)`	⬆️
python/cudf/cudf/core/dtypes.py	`91.13% <100.00%> (+1.40%)`	⬆️
... and 55 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5fea6ad...54395f0. Read the comment docs.


          Add a metadata for offset column of array type.

6adafda

This is required by native.

Signed-off-by: Firestarman <firestarmanllc@gmail.com>

firestarman added the improvement label

firestarman mentioned this pull request

[BUG] Fail to convert the data to arrow format when there is a child column of struct type. #7570

Closed

firestarman marked this pull request as ready for review

March 16, 2021 01:58

firestarman requested a review from a team as a code owner

March 16, 2021 01:58

Contributor Author

firestarman commented Mar 16, 2021

rerun tests

firestarman added the non-breaking label

firestarman mentioned this pull request

Support running scalar pandas UDF with array type. NVIDIA/spark-rapids#1944

Merged

jlowe changed the title ~~JNI: Pass the names of children struct columns down to the native Arrow IPC writer [skip ci].~~ JNI: Pass names of children struct columns to native Arrow IPC writer [skip ci]

jlowe reviewed

View reviewed changes

java/src/main/native/src/TableJni.cpp Outdated Show resolved Hide resolved

java/src/main/native/src/TableJni.cpp Outdated Show resolved Hide resolved

java/src/main/native/src/TableJni.cpp Outdated Show resolved Hide resolved


          Use the regular JNI call to create the c++ column meta

44fa204

Do this to avoid callback callback into the JVM.

Signed-off-by: Firestarman <firestarmanllc@gmail.com>

github-actions bot added the CMake label

firestarman added 2 commits

March 17, 2021 16:16


          Correct the year

a767e70

Signed-off-by: Firestarman <firestarmanllc@gmail.com>


          comment update

d83d0e9

Signed-off-by: Firestarman <firestarmanllc@gmail.com>

jlowe requested changes

View reviewed changes

Member

jlowe left a comment

It's not necessary to refactor this to the flattened name approach, but there are some resource leaks that are possible with the approach used here that are not with the flattened approach and should be fixed.

java/src/main/java/ai/rapids/cudf/ColumnMetadata.java Outdated Show resolved Hide resolved

java/src/main/native/src/ColumnMetadataJni.cpp Outdated Show resolved Hide resolved

java/src/main/native/src/ColumnMetadataJni.cpp Outdated Show resolved Hide resolved

java/src/main/native/src/TableJni.cpp Outdated Show resolved Hide resolved

java/src/main/native/src/TableJni.cpp Outdated Show resolved Hide resolved


          Align with the flattened name approach.

4c2eeef

Signed-off-by: Firestarman <firestarmanllc@gmail.com>

github-actions bot removed the CMake label


          Return a reference from get_column_name

fbbbe5b

Signed-off-by: Firestarman <firestarmanllc@gmail.com>

Contributor Author

firestarman commented Mar 18, 2021 •

edited

Loading

It's not necessary to refactor this to the flattened name approach, but there are some resource leaks that are possible with the approach used here that are not with the flattened approach and should be fixed.

I updated to align with the flattened name approach, and it is a good suggestion, because it not only reduces the code change, but also hides some column metadata details (e.g. stub meta for list type) from Java.

Contributor Author

firestarman commented Mar 18, 2021

rerun tests

firestarman added 4 commits

March 18, 2021 15:55


          Update the function signaure

bc33abc

Signed-off-by: Firestarman <firestarmanllc@gmail.com>


          correct the child index of array type column

eb39f16

Signed-off-by: Firestarman <firestarmanllc@gmail.com>


          Correct the index for list type

55a1e17

Signed-off-by: Firestarman <firestarmanllc@gmail.com>


          Comment update

07d09e5

Signed-off-by: Firestarman <firestarmanllc@gmail.com>


          Remove unexpected rmm log file

87d403a

Signed-off-by: Firestarman <firestarmanllc@gmail.com>

jlowe reviewed

View reviewed changes

Member

jlowe left a comment

Thanks for updating @firestarman, this did get a lot cleaner overall. The main thing I see missing now is that the behavior of column names for nested types isn't documented in the Java APIs anywhere. If we're going the flattened names route for all writers then this should be documented on WriterBuilder, but if the flattening logic is only going to apply to Arrow IPC then its builder should override the withColumnNames method if only to provide documentation on the expected behavior.

cc: @revans2 for visibility

java/src/main/native/src/TableJni.cpp Outdated Show resolved Hide resolved

java/src/main/native/src/TableJni.cpp Outdated Show resolved Hide resolved

java/src/main/native/src/TableJni.cpp Show resolved Hide resolved

java/src/main/native/src/TableJni.cpp Show resolved Hide resolved


          Address some comments

1f21c3a

Signed-off-by: Firestarman <firestarmanllc@gmail.com>

Contributor Author

firestarman commented Mar 19, 2021 •

edited

Loading

Thanks for updating @firestarman, this did get a lot cleaner overall. The main thing I see missing now is that the behavior of column names for nested types isn't documented in the Java APIs anywhere. If we're going the flattened names route for all writers then this should be documented on WriterBuilder, but if the flattening logic is only going to apply to Arrow IPC then its builder should override the withColumnNames method if only to provide documentation on the expected behavior.

I think it is only for Arrow IPC now, so I updated its builder to override the two withXXXXNames for the documentation.

firestarman added 3 commits

March 19, 2021 10:28


          error message update

420f524

Signed-off-by: Firestarman <firestarmanllc@gmail.com>


          Only root columns and nested struct columns consume names

b8966a0

Signed-off-by: Firestarman <firestarmanllc@gmail.com>


          error message update

54395f0

Signed-off-by: Firestarman <firestarmanllc@gmail.com>

jlowe approved these changes

View reviewed changes

Contributor Author

firestarman commented Mar 20, 2021

@gpucibot merge

firestarman added the 5 - Ready to Merge label

Contributor Author

firestarman commented Mar 20, 2021

@gpucibot merge

rapids-bot bot merged commit cdd44d2 into rapidsai:branch-0.19

firestarman deleted the arrow_ipc_column_metadata branch

March 20, 2021 01:18

Contributor Author

firestarman commented Mar 20, 2021

Thanks Jason, learnt a lot

firestarman added a commit to NVIDIA/spark-rapids that referenced this pull request


          Support running scalar pandas UDF with array type. (#1944)

892b4c5

This PR is to support running scalar pandas UDF with array type.

Add array type signature for related expressions and plans.
Flatten the names of nested struct columns from schema, which is also required by the cudf Arrow IPC writer.
This PR depends on rapidsai/cudf#7598

closes #1912

Signed-off-by: Firestarman <firestarmanllc@gmail.com>

nartal1 pushed a commit to nartal1/spark-rapids that referenced this pull request


          Support running scalar pandas UDF with array type. (NVIDIA#1944)

939cb0d

This PR is to support running scalar pandas UDF with array type.

Add array type signature for related expressions and plans.
Flatten the names of nested struct columns from schema, which is also required by the cudf Arrow IPC writer.
This PR depends on rapidsai/cudf#7598

closes NVIDIA#1912

Signed-off-by: Firestarman <firestarmanllc@gmail.com>

nartal1 pushed a commit to nartal1/spark-rapids that referenced this pull request


          Support running scalar pandas UDF with array type. (NVIDIA#1944)

96af496

This PR is to support running scalar pandas UDF with array type.

Add array type signature for related expressions and plans.
Flatten the names of nested struct columns from schema, which is also required by the cudf Arrow IPC writer.
This PR depends on rapidsai/cudf#7598

closes NVIDIA#1912

Signed-off-by: Firestarman <firestarmanllc@gmail.com>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

5 - Ready to Merge improvement Java non-breaking